v8: Fix libwee8.a dropping symbols from duplicate-basename objects - #5138
Merged
Conversation
✅ Deploy Preview for nifty-bassi-e26446 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
phlax
reviewed
Aug 20, 2026
phlax
left a comment
phlax
left a comment
Member
There was a problem hiding this comment.
posting bot review
Things I'd want confirmed
- excluded (exclude_lib_prefixes) is now matched against object paths rather than library paths. If those strings were written to match libabseil…a/libicu…a filenames, they'll no longer match …/absl/base/…/foo.pic.o and abseil/icu objects will silently get pulled into libwee8.a (bloat + ODR/duplicate-symbol risk for consumers). Worth verifying the actual attr values match on directory components (e.g. com_google_absl, icu), not archive names.
- objs = lib.pic_objects if lib.pic_objects else lib.objects — for a library that has both, this takes only PIC, which is intended; but if a dep is alwayslink or splits objects across the two lists, non-PIC objects are dropped without a diagnostic. A comment or an explicit assertion would help the next reader.
- The params file is written with "\n".join(...) and no trailing newline. Both llvm-ar and GNU ar handle this, but a trailing \n is free insurance.
- ar Drcs + @params produces an archive with duplicate member names; ar t output is now ambiguous and ar x on the result would clobber again. Might be worth a one-line note in the rule doc so nobody "fixes" this later.
The fat-archive rule extracted each dep's .lo/.a with `ar x` and re-archived the members by basename. V8 emits 12 objects that share a basename with a twin compiled from a same-named source in another directory (heap, factory, allocation, free-list, sweeper, assembler, utils, objects-printer, ieee754, logging, platform, snapshot). Extracting to the filesystem clobbered one object of each pair, so libwee8.a carried 1158 members but only 1146 distinct contents -- silently dropping every symbol those 12 objects defined (v8::internal::VirtualMemory::~, Factory::NewSymbol, Heap::CollectAllGarbage, HeapObjectIterator, GetCurrentStackPosition, ...). Consumers such as Envoy then failed to link with undefined-symbol errors for exactly those symbols. Collect the distinct object Files from each LibraryToLink directly (pic_objects, falling back to objects), dedup by full path so both twins are kept, exclude abseil/icu by object path, and archive them straight into libwee8.a via `ar Drcs OUT @params`. The result carries duplicate member basenames exactly like V8's own .lo, which links correctly because the linker resolves members by content, not filesystem name. The @file response file avoids ARG_MAX with ~1200 objects. Verified: x86_64 libcxx defined symbols 91,736 -> 111,541 and aarch64 libcxx independently. Signed-off-by: Dario Cillerai <dcillera@redhat.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dario Cillerai <dcillera@redhat.com>
Signed-off-by: Dario Cillerai <dcillera@redhat.com>
dcillera
force-pushed
the
fix-link-undefined
branch
from
August 20, 2026 14:43
739ab9b to
f10e743
Compare
Contributor
Author
Point 1 is verified safe — and the reviewer's worry doesn't apply to our actual attr values. A comment will be added for clarification. Evidence:
The other requests will be addressed in an additional commit:
|
Documentation and robustness follow-ups from PR envoyproxy#5138 review; no change to the archive contents (still 1158 objects, 12 intentional basename collisions). - Document that exclude_lib_prefixes matches OBJECT paths and that the defaults ("abseil-cpp+", "icu+") are the canonical bzlmod repo directory components, so they match every abseil/icu object. Verified: all 99 abseil objects contain "abseil-cpp+"; the noicu build pulls in zero icu objects. - Explain that pic_objects/objects are the same TUs compiled two ways (not a partition), so selecting one list is correct and PIC is preferred. - Write the params file with a trailing newline as insurance for archivers that expect newline-terminated entries. - Expand the rule doc to warn that duplicate member basenames are intentional and required (matches V8's own .lo); do not dedup by basename or extract-and-rearchive (ar x), which reintroduces the symbol-dropping bug. Signed-off-by: Dario Cillerai <dcillera@redhat.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
phlax
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fat-archive rule extracted each dep's .lo/.a with
ar xand re-archived the members by basename. V8 emits 12 objects that share a basename with a twin compiled from a same-named source in another directory (heap, factory, allocation, free-list, sweeper, assembler, utils, objects-printer, ieee754, logging, platform, snapshot). Extracting to the filesystem clobbered one object of each pair, so libwee8.a carried 1158 members but only 1146 distinct contents -- silently dropping every symbol those 12 objects defined (v8::internal::VirtualMemory::~, Factory::NewSymbol, Heap::CollectAllGarbage, HeapObjectIterator, GetCurrentStackPosition, ...). Consumers such as Envoy then failed to link with undefined-symbol errors for exactly those symbols.Collect the distinct object Files from each LibraryToLink directly (pic_objects, falling back to objects), dedup by full path so both twins are kept, exclude abseil/icu by object path, and archive them straight into libwee8.a via
ar Drcs OUT @params. The result carries duplicate member basenames exactly like V8's own .lo, which links correctly because the linker resolves members by content, not filesystem name. The @file response file avoids ARG_MAX with ~1200 objects.Verified: x86_64 libcxx defined symbols 91,736 -> 111,541 and aarch64 libcxx independently.